Reorg introduce api package containing Entry interface by steiler · Pull Request #401 · sdcio/data-server

steiler · 2026-02-19T14:24:12Z

Reorganizes the tree package by introducing a new pkg/tree/api surface and a pkg/tree/consts package, then updates the codebase to use those exported interfaces, types, and constants.

Adds a dedicated API layer (pkg/tree/api) with Entry, LeafVariants, delete-path types, and non-revertive tracking; introduces pkg/tree/ops helpers (e.g., GetByOwner) and an ops interface.
Moves tree constants to pkg/tree/consts and replaces many direct tree.RunningIntentName/RunningValuesPrio/Replace* usages with consts equivalents across datastore, targets, server, and tests.
Refactors core tree implementation to use the new API types: method visibility changes (e.g., ShouldDelete, GetOrCreateChilds, ToJsonInternal), LeafVariants accessors, and TreeContext now exposing SchemaClient, ExplicitDeletes, and NonRevertiveInfo.
Updates mocks and generated code to reflect new API package paths and added interface methods; adds a new mock for SdcpbPath.
Adjusts update/path handling with SdcpbPath() and updates callers, plus broad test updates to match new APIs and constants.

faebr

I really like the interface separation from the stateless ops. I always struggled to fully understand sharedEntryAttributes and this breaks it down! I assume the commented out functions in entry.go are the ones you will still implement right?

faebr · 2026-03-05T12:03:22Z

pkg/tree/api/leaf_variants.go

should the leaf_variants also be parts of the api / interface package or would it make sense to take this implementation into another package so to only have the interface definitions in the api package?

possible... lets add an issue to investigate that. Nothing I would want to also squeeze into this PR.

pkg/tree/ops/deletebranch.go

alexandernorth · 2026-03-10T15:01:47Z

go.mod

 	github.com/sdcio/logger v0.0.3
 	github.com/sdcio/schema-server v0.0.34
-	github.com/sdcio/sdc-protos v0.0.50
+	github.com/sdcio/sdc-protos v0.0.51-0.20260304120533-f0e62d96331e


I guess sdc-protos should be tagged and this updated before this PR is merged

yes exactly

alexandernorth · 2026-03-10T15:21:52Z

pkg/datastore/types/transaction_intent.go

 	explicitDeletes         *sdcpb.PathSet
 	previouslyApplied       bool
+	// revertPaths paths that are meant to be reverted. This is used for non-revertive intents.
+	revertPaths []*sdcpb.Path


We should probably discuss if we want a slice of pointers - imo we should avoid it where possible but that probably is a bigger job to refactor

maybe open an issue for this

alexandernorth · 2026-03-10T15:26:19Z

pkg/datastore/deviations.go

 		}

-		deviationTree.GetDeviations(ctx, deviationChan)
+		dpc := ops.NewGetDeviationConfig(deviationChan)


I think that if we are creating a config to be passed to a processor we should use the config struct directly as it is self documenting, and follows go more closely

alexandernorth · 2026-03-10T16:10:50Z

pkg/datastore/sync.go

 		}
 		// apply delete marker, setting owner delete flag on running intent
-		err = tree.NewOwnerDeleteMarker(tree.NewOwnerDeleteMarkerTaskConfig(tree.RunningIntentName, false)).Run(deleteRoot, d.taskPool)
+		err = processors.NewOwnerDeleteMarker(processors.NewOwnerDeleteMarkerTaskConfig(consts.RunningIntentName, false)).Run(deleteRoot, d.taskPool)


same as previous - here it makes it hard to know what false means, but using the struct directly would give some documentation

pkg/tree/ops/getschemakeys.go

alexandernorth · 2026-03-12T13:11:38Z

pkg/tree/ops/getdeletes.go

+)
+
+// GetDeletes calculate the deletes that need to be send to the device.
+func GetDeletes(e api.Entry, deletes types.DeleteEntriesList, aggregatePaths bool) (types.DeleteEntriesList, error) {


why do we accept the deletes list as an input here?

It's ment to be a recursive function... To reduce allocation of DeleteEntriesList the result list is meant to be carried forward.
refactored with an internal function that takes that params list and the external call that inits an empty list.

alexandernorth · 2026-03-12T15:06:03Z

pkg/tree/ops/getdeviations.go

+	// if s is a presence container but has active childs, it should not be treated as a presence
+	// container, hence the leafvariants should not be processed. For presence container with
+	// childs the TypedValue.empty_val in the presence container is irrelevant.
+	if dt.entry.GetSchema().GetContainer().GetIsPresence() && len(dt.entry.GetChilds(types.DescendMethodActiveChilds)) > 0 {


refactor dt.entry.GetChilds() out to remove additional recomputation as it is also needed later on

alexandernorth · 2026-03-12T15:34:03Z

pkg/tree/ops/proto.go

Aren't there also proto functions in other files too? Other duplicates should probably be removed

No only the adapters that utilize these functions and adapt to the different interfaces

pkg/tree/sharedEntryAttributes_test.go

…centralize dispatch - extract entry validators into dedicated files (range, length, pattern, mandatory, min/max-elements) - keep validate package entrypoint focused on orchestration/result aggregation - introduce dedicated validator dispatch/registry file with shared ValidationFunc signature - precompute active validators once from config and pass through recursive processor tasks - simplify recursive validation path by executing only active validator functions per entry

codecov · 2026-03-13T15:55:29Z

Codecov Report

❌ Patch coverage is 45.38240% with 757 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pkg/utils/testhelper/testhelpers.go	0.00%	108 Missing ⚠️
pkg/tree/ops/treeexport.go	0.00%	58 Missing ⚠️
.../tree/ops/validation/validation_entry_mandatory.go	15.38%	42 Missing and 2 partials ⚠️
pkg/tree/api/childMap.go	31.14%	42 Missing ⚠️
pkg/tree/ops/getorcreatechilds.go	33.87%	37 Missing and 4 partials ⚠️
pkg/tree/ops/getdeletes.go	0.00%	37 Missing ⚠️
pkg/tree/api/choice_case_resolver.go	0.00%	33 Missing ⚠️
pkg/tree/ops/getdeviations.go	0.00%	29 Missing ⚠️
pkg/tree/ops/deletebranch.go	0.00%	28 Missing ⚠️
pkg/datastore/transaction_rpc.go	52.94%	22 Missing and 2 partials ⚠️
... and 52 more

📢 Thoughts on this report? Let us know!

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

steiler requested a review from a team as a code owner February 19, 2026 14:24

steiler added the safe to test label Feb 24, 2026

steiler added 6 commits February 24, 2026 13:45

move entry and related to api package

a27eedb

adjust makefile referencing path

ed87e06

GetByOwner to ops and nonRevertiveInfo fix

efa72f3

add mock forsdcpbpath inteface

9bd0cfe

wire nonrevertive paths

3492c4b

adjust go mod & fix test

651c414

steiler force-pushed the reorgIntroduceAPI branch from 08c3843 to 651c414 Compare February 24, 2026 12:46

steiler added 5 commits February 27, 2026 10:46

reorg

a5f59f7

split further

7f8fe08

update

28b180f

fill new KeyName attribute of BlameTreeElement

0566ae3

GetDeviations use pool

c8a5bcd

faebr reviewed Mar 5, 2026

View reviewed changes

alexandernorth reviewed Mar 12, 2026

View reviewed changes

steiler added 3 commits March 13, 2026 11:38

address comments part1

2355491

fix updating revertiveness

eddffb1

steiler force-pushed the reorgIntroduceAPI branch from e2324c9 to eddffb1 Compare March 13, 2026 15:54

Conversation

steiler commented Feb 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

faebr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Mar 13, 2026

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

steiler commented Feb 19, 2026 •

edited

Loading